Appeng 4536 multi prompt/model factory#267
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
gnetanel
force-pushed
the
APPENG-4536_multi_promp_factory
branch
from
84e151a to
026e556
Compare
There was a problem hiding this comment.
Hi @heatherzh01 @gnetanel @etsien , Good job all!.
There are still some gaps:
- Code Understanding sub-agent in the agent loop was left untouched ( with all of its components), means that if the gemma/granite models will be configured in agent config, it will still use the hardcoded prompts defined in code understanding sub-agent ( only tested on legacy Llama-3.1), So this is an inconsistency as some parts of the checklist items are being routed and dispatched to the code understanding sub-agent, and not to the reachability sub-agent. might not work well with conjunction to other parts and stages that are using the multi prompts factory prompts' based on the configured model in the agent configuration.
- The dispatcher component in the agent loop, which route a checklist item to the appropriate sub-agent, also stayed untouched.
Same meanings and consequences as the above.
-
RPM Analysis is exempted from this enhancement, worthwhile considering if there is a necessity in apply this multi prompt factory to the RPM Agent as a future enhancement in the future ( POST GA enhancement).
-
You should also make rebase on top of main, to resolve current conflicts, and also to catch up with month and half of new commits added to main, another testing cycle should be conducted afterwards.
-
Please see the rest of my comments in the code.
| if language is not None: | ||
| if not isinstance(language, LanguageAdjustments): | ||
| try: | ||
| language = Language(language) |
There was a problem hiding this comment.
@heatherzh01 Language class is not defined anyware, This will raise a NameError exception that is not handled and will be thrown/propagated to upper levels and callers.
|
|
||
| llm = await builder.get_llm(llm_name=config.llm_name, wrapper_type=LLMFrameworkEnum.LANGCHAIN) | ||
|
|
||
| if config.model_family: |
There was a problem hiding this comment.
@heatherzh01 _PKG_GATE and _REACH_GATE are imported only from granite, regardless of the config.model_family being used.
| lang = None | ||
|
|
||
| if _prompt is None: | ||
| _prompt = get_prompt(PromptId.CHECKLIST_MAIN, llm=llm_family_name, language=lang) |
There was a problem hiding this comment.
@heatherzh01 if get_prompt fails and exception PromptCatalogError is raised, it's unhandled error that will crash the pipeline.... ( it will happen if the family name is not supported , or language is not supported or the promptId is invalid - since the checklist prompt is preconfigued, then surprises are possible only from the configurable value(llm family name) and request scoped value ( language).
I Think we should catch this error and gracefully fallback to some default ( alongside an appropriate warning log message, pretty similar to what you did in the code portion above that you commented out).
| @@ -822,6 +636,13 @@ def _build_tool_arguments(actions: ToolCall)->dict[str, Any]: | |||
| return {"query": actions.query} | |||
| if actions.tool_input: | |||
| return {"query": actions.tool_input} # fallback | |||
| # Last resort: use reason as query | |||
There was a problem hiding this comment.
@heatherzh01 Why?
The reason field contains free-text justification like "Check if vulnerable function is present". Passing this as a literal query to Code Keyword Search or Function Locator returns irrelevant results that the agent might consider as and processes as genuine findings — potentially leading to wrong conclusions.
The previous behavior (the raising ValueError fallback) was actually safer: it sent an error message back to the thought node, which then retried with corrected arguments/parameters. ( This way restrained the model to recalculate and check for a better path rather than going on a meaningless path that might be considered legitimate by mistake).
…endering (#235) Introduce a catalog-driven prompt resolver so vulnerability-analysis prompts can be selected and customized by prompt type, LLM family/version, and programming language without scattering template strings across callers. Co-authored-by: Gal Netanel <gnetanel@redhat.com>
…the module itself (#239) Co-authored-by: Gal Netanel <gnetanel@redhat.com>
…amily and version
… format and minor renaming not to clash language class with other common modules
…is being searched
includes a cursor .mdc file outlining the requirements, setup, and instructions for use (requires a corresponding exploitiq-tests-automation repo to be installed alongside)
It is not called though, because need to figure how to get llm info , see comments for info
A seperate commit has been created for the main branch. In multi prompt feature we generate prompt differently, so a fix is required in here as well.
Missing info is based on llama configuration, this is reflected in the string name that keep the prompt text
heatherzh01
force-pushed
the
APPENG-4536_multi_promp_factory
branch
from
d136ac7 to
c094afb
Compare
5 participants
introducing multi-prompt factory with different configs for different models (gemma, granite, or llama), replacing previous single shared prompt for all models, each model get its own tailored prompts tuned to reasoning styles. Switching models only requires config changes, system will route to correct prompt catalog accordingly.